-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[flang][Parser] Convert applyMem to invoke non-void members #119782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Currently applyMem(f, a, ...) calls a.f(...), but still returns the
result of parser a. This is in contrast to applyFunction(f, a, ...),
which returns the result of calling f(a, ...).
The use case for this is being able to parse quoted or unquoted
strings, and store the result as std::string:
```
construct<IdentifierOrString>(
(space >> charLiteralConstantWithoutKind) ||
applyMem(&Name::ToString, Parser<Name>{})) // Parser<Name>{}.ToString()
```
The applyMem combinator is currently unused.
|
@llvm/pr-subscribers-flang-parser Author: Krzysztof Parzyszek (kparzysz) ChangesCurrently applyMem(f, a, ...) calls a.f(...), but still returns the result of parser a. This is in contrast to applyFunction(f, a, ...), which returns the result of calling f(a, ...). The use case for this is being able to parse quoted or unquoted strings, and store the result as std::string: The applyMem combinator is currently unused. Full diff: https://github.com/llvm/llvm-project/pull/119782.diff 2 Files Affected:
diff --git a/flang/docs/ParserCombinators.md b/flang/docs/ParserCombinators.md
index 7cb77deba21971..076e76f703c49c 100644
--- a/flang/docs/ParserCombinators.md
+++ b/flang/docs/ParserCombinators.md
@@ -141,7 +141,7 @@ collect the values that they return.
* `applyLambda([](&&x){}, p1, p2, ...)` is the same thing, but for lambdas
and other function objects.
* `applyMem(mf, p1, p2, ...)` is the same thing, but invokes a member
- function of the result of the first parser for updates in place.
+ function of the result of the first parser.
### Token Parsers
Last, we have these basic parsers on which the actual grammar of the Fortran
diff --git a/flang/lib/Parser/basic-parsers.h b/flang/lib/Parser/basic-parsers.h
index 515b5993d67376..1a8c14e7048f64 100644
--- a/flang/lib/Parser/basic-parsers.h
+++ b/flang/lib/Parser/basic-parsers.h
@@ -580,11 +580,11 @@ template <typename PA> inline constexpr auto defaulted(PA p) {
// applyLambda(f, ...) is the same concept extended to std::function<> functors.
// It is not constexpr.
//
-// Member function application is supported by applyMem(f, a). If the
-// parser a succeeds and returns some value ax, the result is that returned
-// by ax.f(). Additional parser arguments can be specified to supply their
-// results to the member function call, so applyMem(f, a, b) succeeds if
-// both a and b do so and returns the result of calling ax.f(std::move(bx)).
+// Member function application is supported by applyMem(&C::f, a). If the
+// parser a succeeds and returns some value ax of type C, the result is that
+// returned by ax.f(). Additional parser arguments can be specified to supply
+// their results to the member function call, so applyMem(&C::f, a, b) succeeds
+// if both a and b do so and returns the result of calling ax.f(std::move(bx)).
// Runs a sequence of parsers until one fails or all have succeeded.
// Collects their results in a std::tuple<std::optional<>...>.
@@ -654,39 +654,31 @@ inline /* not constexpr */ auto applyLambda(
}
// Member function application
-template <typename OBJPARSER, typename... PARSER> class AMFPHelper {
- using resultType = typename OBJPARSER::resultType;
-
-public:
- using type = void (resultType::*)(typename PARSER::resultType &&...);
-};
-template <typename OBJPARSER, typename... PARSER>
-using ApplicableMemberFunctionPointer =
- typename AMFPHelper<OBJPARSER, PARSER...>::type;
-
-template <typename OBJPARSER, typename... PARSER, std::size_t... J>
-inline auto ApplyHelperMember(
- ApplicableMemberFunctionPointer<OBJPARSER, PARSER...> mfp,
- ApplyArgs<OBJPARSER, PARSER...> &&args, std::index_sequence<J...>) ->
- typename OBJPARSER::resultType {
- ((*std::get<0>(args)).*mfp)(std::move(*std::get<J + 1>(args))...);
- return std::get<0>(std::move(args));
+template <typename MEMFUNC, typename OBJPARSER, typename... PARSER,
+ std::size_t... J>
+inline auto ApplyHelperMember(MEMFUNC mfp,
+ ApplyArgs<OBJPARSER, PARSER...> &&args, std::index_sequence<J...>) {
+ return ((*std::get<0>(args)).*mfp)(std::move(*std::get<J + 1>(args))...);
}
-template <typename OBJPARSER, typename... PARSER> class ApplyMemberFunction {
- using funcType = ApplicableMemberFunctionPointer<OBJPARSER, PARSER...>;
+template <typename MEMFUNC, typename OBJPARSER, typename... PARSER>
+class ApplyMemberFunction {
+ static_assert(std::is_member_function_pointer_v<MEMFUNC>);
+ using funcType = MEMFUNC;
public:
- using resultType = typename OBJPARSER::resultType;
+ using resultType =
+ std::invoke_result_t<MEMFUNC, typename OBJPARSER::resultType, PARSER...>;
+
constexpr ApplyMemberFunction(const ApplyMemberFunction &) = default;
- constexpr ApplyMemberFunction(funcType f, OBJPARSER o, PARSER... p)
+ constexpr ApplyMemberFunction(MEMFUNC f, OBJPARSER o, PARSER... p)
: function_{f}, parsers_{o, p...} {}
std::optional<resultType> Parse(ParseState &state) const {
ApplyArgs<OBJPARSER, PARSER...> results;
using Sequence1 = std::index_sequence_for<OBJPARSER, PARSER...>;
using Sequence2 = std::index_sequence_for<PARSER...>;
if (ApplyHelperArgs(parsers_, results, state, Sequence1{})) {
- return ApplyHelperMember<OBJPARSER, PARSER...>(
+ return ApplyHelperMember<MEMFUNC, OBJPARSER, PARSER...>(
function_, std::move(results), Sequence2{});
} else {
return std::nullopt;
@@ -698,11 +690,11 @@ template <typename OBJPARSER, typename... PARSER> class ApplyMemberFunction {
const std::tuple<OBJPARSER, PARSER...> parsers_;
};
-template <typename OBJPARSER, typename... PARSER>
+template <typename MEMFUNC, typename OBJPARSER, typename... PARSER>
inline constexpr auto applyMem(
- ApplicableMemberFunctionPointer<OBJPARSER, PARSER...> mfp,
- const OBJPARSER &objParser, PARSER... parser) {
- return ApplyMemberFunction<OBJPARSER, PARSER...>{mfp, objParser, parser...};
+ MEMFUNC memfn, const OBJPARSER &objParser, PARSER... parser) {
+ return ApplyMemberFunction<MEMFUNC, OBJPARSER, PARSER...>{
+ memfn, objParser, parser...};
}
// As is done with function application via applyFunction() above, class
|
klausler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a use of the new capabilities in the parser or some kind of test case.
| and other function objects. | ||
| * `applyMem(mf, p1, p2, ...)` is the same thing, but invokes a member | ||
| function of the result of the first parser for updates in place. | ||
| function of the result of the first parser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the new version no longer work for updates in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns the value returned by the member call, so for member functions returning void (as it was before) it wouldn't work. For member functions returning reference to *this or something equivalent it should be fine.
Is modifying in-place is still needed? If so I'd need to separate the code from this PR into its own parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to look at usage sites (of which there are not many) to be sure this comment is obsolete. I wouldn't have added that comment unless I had been thinking about mutation in situ at the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any uses of this in the upstream sources. I thought that maybe it's used in some downstream project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it, then. Downstream usage can adapt if needed.
Will do. PS. Technically I'm on vacation for the end of the year, so my replies or updates may be sparser than usual. |
|
Decided against it in the end. There would only be a single use case for this at the moment, and it will likely go away, since that code may need updates in the future. |
Currently applyMem(f, a, ...) calls a.f(...), but still returns the result of parser a. This is in contrast to applyFunction(f, a, ...), which returns the result of calling f(a, ...).
The use case for this is being able to parse quoted or unquoted strings, and store the result as std::string:
The applyMem combinator is currently unused.